The comment fairly strongly suggests that it does it for a reason though… will this cause, eg. all messages from people who've left to appear with no display name / avatar? Or is this fine because that's kept in the sentinel events? Do all clients actually use them reliably?

Also, I know you've not written any new code here but some tests could go a long way to help set out what we think should happen, eg: "member event for left user should not contain display name", "member event on historical message for left user should contain display name" etc.

The comment fairly strongly suggests that it does it for a reason though… will this cause, eg. all messages from people who've left to appear with no display name / avatar? Or is this fine because that's kept in the sentinel events? Do all clients actually use them reliably?

I suspect that this code was originally added for the convenience of client developers, so they wouldn't have to add extra logic when looking up the display names and avatars of users which are no longer in the room. However, it also removed the client's ability to choose if those users should have their display names and avatars visible. I believe that deciding if and when to show that data should be the responsibility of the client, not the SDK, because having the SDK silently add the fields back in can cause surprising behavior. In fact, I opened this PR after discovering that Cinny showed the abusive avatar of a banned troll even though the troll's join event was redacted.

To be more specific, here's the scenario I observed:

1. troll joins and sends spam messages
2. room admin redacts the messages, bans the troll, and redacts the troll's join event
3. Cinny shows the redacted messages with the troll's abusive avatar, which it reads from the prev_content field of the m.room.member state event that banned the troll

matrix-js-sdk's getMxcAvatarUrl function appears to be intended to return the latest avatar a user has in a room. (As far as I can tell, the SDK does not implement the algorithm suggested in 10.2.2.4 for displaying historical display names and avatars.) The current behavior returns the last avatar the user had set in the room, which is different from 10.2.2.4's suggested algorithm because it ignores redactions to older state events. When a user leaves or is banned from a room, they no longer have a latest avatar, so this function should return undefined, which is the behavior this PR implements and I believe to be closer to what the spec recommends.

It might be desirable in the future for matrix-js-sdk to include a way to return historical avatars and display names in a way that respects redactions, as described in 10.2.2.4.

The js-sdk's current behaviour does seem a bit dumb here. While returning no avatar / display name for left users might be argued to be technically correct, if it's going to cause clients to suddenly start displaying left users with no display name or avatar, I don't think this is a thing we can just hit merge on. It would at least be a breaking change. If possible, fixing getMxcAvatarUrl's current behaviour to respect redactions might be preferable?

The js-sdk's current behaviour does seem a bit dumb here. While returning no avatar / display name for left users might be argued to be technically correct, if it's going to cause clients to suddenly start displaying left users with no display name or avatar, I don't think this is a thing we can just hit merge on. It would at least be a breaking change. If possible, fixing getMxcAvatarUrl's current behaviour to respect redactions might be preferable?

As written this is currently a breaking change, yeah. I'll investigate fixing getMxcAvatarUrl.

Did you get a chance to take a look at this? Just because we'll close this PR if it's not actively being worked on (we can re-open later of course).

Did you get a chance to take a look at this? Just because we'll close this PR if it's not actively being worked on (we can re-open later of course).

I got distracted by other things, sorry! I'll take a look at it today.

After investigating further, I have determined that the behavior I was observing is likely a bug with the continuwuity homeserver incorrectly returning prev_content for redacted state events, since on Synapse my client doesn't show avatars for banned users with redacted join events. I've opened a bug upstream here. I'll keep this PR open for now.

This keeps coming up in our weekly syncs as an outstanding PR, so I'm going to close it for the time being. We can always reopen it if needed.